Support connecting to an RCP over a raw TCP socket#39
Conversation
a48851e to
0a52be7
Compare
puddly
left a comment
There was a problem hiding this comment.
Looks good to me. Have you gotten it to run with a live device?
Yes, it was on a version based on the code before the esp32 support was committed. I had issues with dev at the time - which leads me to: what version of HA is dev working with? Stuck in the office today - I can test this last change at some point ›later tonight. Hardware flow between on the uart line between the esp32 and the rcp is essential though. without I think it won't be nearly as stable as I've seen it (across this and my thread network on another tcp connected rcp) . |
HA Core hasn't been updated yet to work with any of the recent changes. You will need to manually upgrade |
ziggurat-spinel's SpinelClient was hard-coded to tokio_serial::SerialStream. Generalize it to a boxed RcpTransport (any AsyncRead + AsyncWrite), and teach ziggurat-server to open a TcpStream when --device is given as tcp://host:port, falling back to the existing serial-port path otherwise. Rebased onto dev (through zigpy#46, "Granular network state events for backup/restore"). The wire protocol was rewritten from JSON-over-WebSocket to a COBS-framed binary protocol (zigpy#38, ziggurat-protocol) since this was last rebased; only ziggurat-spinel/src/client.rs and the transport-opening half of ziggurat-server/src/main.rs (SerialConfig, open_transport, fn phy and its callers) needed re-applying — the request/response dispatch layer itself was untouched by this change and is left as upstream has it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: puddly <32534428+puddly@users.noreply.github.com>
8caf1ea to
a73c330
Compare
|
rebased today and testing with zigpy-ziggurat pr#3 able to connect and control devices. I can PR the addon update too: https://github.com/tube0013/addons/tree/tcp-adaptor-test if you accept this one.
|
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the RCP-over-TCP support. The shape is good: a RcpTransport marker trait with a blanket impl is the minimal way to make SpinelClient transport-agnostic, open_transport keeps the URL handling in one place, set_nodelay is the right call for small request/response frames, and a failed open isn't cached (*phy stays None on the ?), so a transient TCP connect failure is retried by the next command instead of wedging the process.
Verified, no action needed:
- Dropping
#[derive(Debug)]fromSpinelClient/SpinelWriteris safe —SpinelPhydoesn't deriveDebugand nothing else formats the client. Box<dyn RcpTransport>satisfiestokio::io::split: supertrait elaboration givesdyn RcpTransport: AsyncRead + AsyncWrite + Unpin, and tokio implements both traits forBox<T>.map_err(std::io::Error::other)flattens theErrorKindtoOtherbut preserves the message, and the only consumer isradio_error, which stringifies — no user-visible regression.- The
std::sync::Mutex→tokio::sync::Mutexswap onphyis forced (the guard now spans an await) and can't deadlock:phy()is the only place that takes that lock. - Nothing depended on serial control lines (RTS/DTR), so boxing the transport loses no capability.
- A dropped TCP socket is recoverable the same way a USB yank is — the reader loop hits EOF and exits the process for the supervisor to restart.
CI's prek job runs cargo check --workspace, cargo build and cargo clippy --all-targets --all-features -D warnings -Dclippy::nursery, and it passed — so compilation and lints are covered here. (There's no cargo test hook in the pre-commit config, so tests aren't exercised in CI at all; that's pre-existing, not something this PR introduces.)
Four non-blocking points inline — the first two are the substantive ones. One more that isn't anchorable to a changed line: crates/ziggurat-spinel/src/client.rs:313 and :317 log "Serial port EOF, exiting" and "Serial port read failed ({e}), exiting". Those are now the TCP-disconnect path too, and "serial port" will misdirect anyone debugging a network-attached RCP — worth generalising to something like "RCP link" while this is fresh.
Addresses two of zigpy-review-bot's review comments on zigpy#39: - open_transport's TcpStream::connect had no timeout. Against a host that blackholes SYNs (a powered-off ser2net bridge, a firewall dropping rather than rejecting), the connect rides out the kernel's SYN-retry window (minutes) while phy()'s lock is held, wedging every other command behind it for that whole time. Bounded to 10s. - Accept socket:// alongside tcp://: cheap, and directly guards against the exact failure mode already hit once (an unrecognized URL scheme silently falling through to the serial branch and failing with a confusing ENOENT instead of a clear error). socket:// is a long-established convention in the pyserial ecosystem for a TCP-bridged serial device, so it's a natural first guess for anyone used to that world. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Separate commit from the timeout/socket:// fix so it's easy to back out or revise independently based on further review. Addresses zigpy-review-bot's remaining two comments on zigpy#39: the phy field/ fn phy doc comments and the TIMEOUT comment on SpinelClient still described the transport as strictly serial (including an EBUSY rationale that doesn't apply to a TCP socket), reading narrower than the code now is. Also states explicitly, in the open_transport doc comment and the --device CLI help, what was previously only implicit: the tcp:// transport is for a wired (Ethernet) link. Wi-Fi is not recommended or supported — its latency variance can spuriously trip MAX_CONSECUTIVE_TIMEOUTS and force a reset-recovery against a perfectly healthy radio. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Worked through the bot's comments with Claude and all changes were approved by me before pushing. Originally we had the naming changes but it seemed to make the PR larger than I wanted up front so I had them removed as they were cosmetic to me. let me know if you want any more testing done/related data or changes. |
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Re-reviewed at 3bf94df. All four points from the earlier review are addressed, and I verified each against the tree rather than taking the replies at face value:
- Unbounded TCP connect under the
phylock — fixed in41ec5d6exactly as suggested:CONNECT_TIMEOUT(10 s) wrapsTcpStream::connect, and themap_err(...)?+?pair correctly flattens theElapsedinto anio::Errorand then propagates the connect's own error. The bound covers DNS resolution too, sinceconnecttakes the&strand resolves inside the timeout. The serial path is left unbounded, which is right —open_native_asyncis a localopen(2). socket://alias — added in41ec5d6. Worth recording that tube0013's correction to my rationale is the accurate one:zigpy.serial.create_serial_connectionin zigpy/zigpy-ziggurat#3 backsSpinelTransport, a separate path that talks Spinel straight to the chip and bypasses this server, so it isn't the same config field as--deviceand there was no interop break to fix. The alias still earns its one line as a guard against pyserial habit, just not for the reason I gave.phy()/ field doc andTIMEOUTdoc — both generalized in3bf94df, with theEBUSYrationale broadened to cover a second concurrent connection to the same TCP endpoint, and the Wi-Fi caveat made explicit at all three places a reader might look (TIMEOUT,open_transport,--devicehelp). Declaring the TCP path Ethernet-only is the right call over retuningMAX_CONSECUTIVE_TIMEOUTSblind.
Also re-verified this round:
- No lock-ordering hazard from the
std::sync::Mutex→tokio::sync::Mutexswap onphy. The remainingstacksync-mutex takes (main.rs:141,:610,:658) are all single-expression scopes that drop the guard before any.await, andself.phy.lock()appears only insidephy()itself, so the two locks are never held simultaneously and neither is held across an await outside the deliberate open-serialization. - Reset recovery is transport-agnostic — the
MAX_CONSECUTIVE_TIMEOUTSpath atclient.rs:410only callsnotify_reseton the in-memory protocol state; it never reopens the link, so it behaves identically over TCP. - A second opinion from another model over the same diff returned no findings.
Two leftovers, both cosmetic and non-blocking — neither is anchorable to a changed line, so they're here rather than inline:
crates/ziggurat-spinel/src/client.rs:316and:320still log"Serial port EOF, exiting"and"Serial port read failed ({e}), exiting". This is the same point I raised last time; it's the one thing that didn't get swept up in the3bf94dfwording pass. These two lines are now the TCP-disconnect path as well, and they're what someone debugging a dropped network link will actually see, so "serial port" will misdirect exactly when it's least helpful. Something like "RCP link" would match the vocabulary the rest of the PR just standardized on.crates/ziggurat-server/src/main.rs:55—/// Map a serial-port open failure to a protocol error.onradio_error. Now stale in the same way: it also carries TCP connect andset_nodelayfailures.
Testing caveat: there's no Rust toolchain on this machine, so I read the code rather than building it. Compilation and lints are covered by CI's prek job (cargo check --workspace, cargo build, cargo clippy --all-targets --all-features -D warnings -Dclippy::nursery), which is green at this head. That job runs no cargo test, so the test suite is unexercised in CI — pre-existing, not introduced here. tube0013's live-device testing against zigpy/zigpy-ziggurat#3, with connect-and-control confirmed, covers the path that static review can't.
Approving on the strength of the verified fixes; the two wording nits are free to fold into a follow-up or ignore.
|
Thanks! The cosmetic stuff isn't a problem, we'll clean up the strings at some point anyways. |
|
thanks! |
Adds a network_address config option (tcp://host:port) that takes priority over the device serial-port picker when set, wired through to ziggurat-server's --device flag (zigpy/ziggurat#39, merged). Wi-Fi-connected radios are not recommended. The device field is still required by the add-on's config form even when network_address is set (the form won't validate otherwise), but its value is functionally ignored in that case. Targets the next zigpy/ziggurat release rather than the current 0.1.0 on crates.io, which predates this change; the ZIGGURAT_VERSION build arg will need confirming against whatever version is actually published, since zigpy/ziggurat's publish workflow stamps the version from the release tag rather than from its Cargo.toml. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a network_address config option (tcp://host:port) that takes priority over the device serial-port picker when set, wired through to ziggurat-server's --device flag (zigpy/ziggurat#39, merged). Wi-Fi-connected radios are not recommended. The device field is still required by the add-on's config form even when network_address is set (the form won't validate otherwise), but its value is functionally ignored in that case. Targets the next zigpy/ziggurat release rather than the current 0.1.0 on crates.io, which predates this change; the ZIGGURAT_VERSION build arg will need confirming against whatever version is actually published, since zigpy/ziggurat's publish workflow stamps the version from the release tag rather than from its Cargo.toml. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>



ziggurat-spinel's SpinelClient was hard-coded to tokio_serial::SerialStream. Generalize it to a boxed RcpTransport (any AsyncRead + AsyncWrite), and teach ziggurat-server to open a TcpStream when --device is given as tcp://host:port, falling back to the existing serial-port path otherwise.